Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix/delete read only remnants folders #7061

Merged
merged 5 commits into from
Sep 12, 2024
Merged

Bugfix/delete read only remnants folders #7061

merged 5 commits into from
Sep 12, 2024

Conversation

mgallien
Copy link
Collaborator

@mgallien mgallien commented Sep 2, 2024

No description provided.

@mgallien mgallien force-pushed the bugfix/deleteReadOnlyRemnantsFolders branch from d55760d to 8111dd3 Compare September 3, 2024 06:44
@mgallien
Copy link
Collaborator Author

mgallien commented Sep 3, 2024

popup asking for delete confirmation
Screenshot 2024-09-03 094212

@mgallien
Copy link
Collaborator Author

mgallien commented Sep 3, 2024

@nextcloud/designers can you help with the UI review ? 🙏

@mgallien mgallien force-pushed the bugfix/deleteReadOnlyRemnantsFolders branch 3 times, most recently from 06901c9 to 411c0bd Compare September 4, 2024 13:10
@marcoambrosini
Copy link
Member

@mgallien you mean review of this popup dialog? Are those windows standard components?

@mgallien
Copy link
Collaborator Author

mgallien commented Sep 5, 2024

@mgallien you mean review of this popup dialog? Are those windows standard components?

yes the popup screenshot would show the message
the components are standard ones and should follow the platform style (or way to show this type of popup)

@marcoambrosini I am more concerned by the content of the message and if you have suggestions about it

@marcoambrosini
Copy link
Member

Got it @mgallien, I have to admit I don't understand exactly what's going on there. Could you explain the context with a bit more detail so that I can help with the text? :)

@mgallien
Copy link
Collaborator Author

mgallien commented Sep 9, 2024

/backport to stable-3.14

@mgallien
Copy link
Collaborator Author

mgallien commented Sep 9, 2024

/backport to stable-3.13

@mgallien
Copy link
Collaborator Author

mgallien commented Sep 9, 2024

Got it @mgallien, I have to admit I don't understand exactly what's going on there. Could you explain the context with a bit more detail so that I can help with the text? :)

@marcoambrosini
we had a bug were desktop client could leave folders that are read-only inside a read-only parent folder
those folders should have been deleted
we now detect them and ask the user confirmation before cleaning them up
not cleaning them is causing synchronization errors so the user would probably care and look at the dialog when it pops up

@mgallien mgallien force-pushed the bugfix/deleteReadOnlyRemnantsFolders branch from abd7fc0 to ccb33bb Compare September 10, 2024 13:16
claucambra
claucambra requested changes Sep 11, 2024
View reviewed changes
src/libsync/filesystem.cpp Show resolved Hide resolved
src/libsync/filesystem.cpp Show resolved Hide resolved
src/gui/folder.cpp Outdated Show resolved Hide resolved
jancborchardt
jancborchardt requested changes Sep 11, 2024
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we now detect them and ask the user confirmation before cleaning them up
not cleaning them is causing synchronization errors so the user would probably care and look at the dialog when it pops up

@mgallien @claucambra @camilasan in these cases we should automatically clear it up. Reading this popup I only see possible confusion, and no reason to not click the "Remove remnant folders" button.

We need to do as many things as possible automatically. Every time we ask people something, it can:

  • Be missed
  • Be misunderstood
  • Lead to confusion
  • Lead to frustration why it’s not understandable
  • etc.

(Similar thing with the "spaces at end of filenames" issue for example.)

@marcoambrosini
Copy link
Member

marcoambrosini commented Sep 11, 2024 •
edited
Loading

not cleaning them is causing synchronization errors so the user would probably care and look at the dialog when it pops up

Do you think we can perform this action without asking once we detect that folders that should have been deleted are in fact still there? cc @jancborchardt

edit: just saw your comment after refreshing Jan :)

@mgallien
Copy link
Collaborator Author

@jancborchardt
I agree with your points
I initially added this as I received feedback from some users that were worried about the delete operation
I will remove the dialog and indeed make sure the client is doing its best to ensure a working state with synchronization be in a working state

@mgallien mgallien force-pushed the bugfix/deleteReadOnlyRemnantsFolders branch from ccb33bb to 6407997 Compare September 11, 2024 15:39
@mgallien
Copy link
Collaborator Author

you can review again
i tested with the removal of dialog and it is still working fine

jancborchardt
jancborchardt approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no dialog, all good from my side. :) Thanks @mgallien!

@mgallien
collect remnant read-only folders during discovery
b7b72bf 
Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien
let cancel sync and finish sync lambda be reusable methods
155c61a 
will enable implementation of other ways to interrupt sync after
discovery to get user feedback

Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien
detect remnants read-only folders to delete: try to delete them
a118c0a 
Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien
let FileSystem::removeRecursively be able to delete read-only folders
34c25ec 
Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien
delete invalid folders and restart sync automatically
8faefc6 
users should then notice that the sync errors are gone

Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien mgallien force-pushed the bugfix/deleteReadOnlyRemnantsFolders branch from 6407997 to 8faefc6 Compare September 12, 2024 06:40
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
76.6% Coverage on New Code (required ≥ 80%)
E Reliability Rating on New Code (required ≥ A)
80 New Code Smells (required ≤ 0)
1 New Bugs (required ≤ 0)
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-7061-8faefc6fa28d9a4bf956ec074cc6f9b60975eb88-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@mgallien mgallien enabled auto-merge September 12, 2024 07:29
@mgallien mgallien merged commit 341263b into master Sep 12, 2024
11 of 15 checks passed
@mgallien mgallien deleted the bugfix/deleteReadOnlyRemnantsFolders branch September 12, 2024 07:42
This was referenced Sep 12, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claucambra claucambra claucambra approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@camilasan camilasan Awaiting requested review from camilasan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mgallien @marcoambrosini @nextcloud-desktop-bot @jancborchardt @claucambra